Using wrappedToken in CompositeLiquidityRouter in ERC4626Pool operations #1201

elshan-eth · 2024-12-24T15:46:18Z

Description

The current CLR implementation does not support adding mixed assets. For example, if my pool consists of two wrapped tokens, and I already have one of them and want to use it, this is not possible with the current CLR. The reason is that it treats all input tokens as underlying assets. This PR resolves the issue by introducing a new flag in the interface, allowing us to specify which type of token to use.

Type of change

Checklist:

The diff is legible and has no extraneous changes
Complex code has been commented, including external interfaces
Tests have 100% code coverage
The base branch is either main, or there's a description of how to merge

Issue Resolution

Closes #1192

jubeira

First pass done. I didn't get to the tests yet, but I think there are a few things that need to be adjusted before proceeding.

Overall the idea is correct; great job. It's just that some of the edge cases should behave differently now that we're not automatically wrapping / unwrapping.

pkg/vault/contracts/CompositeLiquidityRouter.sol

jubeira

Another pass done.

Changes are looking good for the most part. Just a few minor comments around names / docs / style; otherwise seems to be correct. I'll finish covering the tests in one more pass. Great job!

pkg/vault/contracts/CompositeLiquidityRouter.sol

EndymionJkb

Looks like a pretty elegant solution (especially to the limits problem). I have a conceptual issue with the useWrappedTokens name, and lots of little details.

I also think these PRs need a lot more in the description. Why do we need it? What problem does it address? What options did we consider and reject (e.g., in this case, the limits hack, replaced by splitting the wrap/unwrap functions)?

Otherwise we're not going to remember all this context when we go back and look at this in 6 months or a year.

pkg/interfaces/contracts/vault/ICompositeLiquidityRouter.sol

pkg/vault/contracts/CompositeLiquidityRouter.sol

Co-authored-by: EndymionJkb <[email protected]>

…ttps://github.com/balancer/balancer-v3-monorepo into using-wrapped-amount-in-erc4626-operations-in-clr

EndymionJkb

I think we need to address the boolean array name (see my suggestion to use isStandardToken or useAsStandardToken), and returning the token addresses, which would resolve all the "sorted in ... order" comment issues.

elshan-eth · 2025-01-14T17:36:29Z

My eyes are already lost in the description, I double checked several times, but if you can double check, I would be very grateful

EndymionJkb

Looking much better! Note the suggested language for NatSpec, which I believe is much clearer.

pkg/interfaces/contracts/vault/ICompositeLiquidityRouter.sol

pkg/vault/contracts/CompositeLiquidityRouter.sol

jubeira · 2025-01-15T13:53:07Z

Sorry for the late comment, but I feel we're making up new terms here 😅

We're already dealing with wrapped and underlying, and that should be enough. I can't know which of them is standard in that context without reading the docs, which is not ideal.

The argument is consumed by a function called _wrapTokensExactInIfRequired or _wrapTokensExactOutIfRequired. Then, a more appropriate name for the array of flags would be wrapUnderlying. It's pretty clear that if an element is true, you wrap the given underlying amount; if not, you don't.

EndymionJkb · 2025-01-15T15:23:12Z

The argument is consumed by a function called _wrapTokensExactInIfRequired or _wrapTokensExactOutIfRequired. Then, a > more appropriate name for the array of flags would be wrapUnderlying. It's pretty clear that if an element is true, you wrap > the given underlying amount; if not, you don't.

Makes sense - especially because there isn't an unwrap, so there's only one name for the set of flags. The only drawback is the "default" value would be to set everything to true (kind of annoying), but I agree that reusing standard terms is more important.

jubeira

This is looking great.

Just a few more comments; otherwise LGTM. I like the new structure much better; it reuses less code, but it's more clear and goes straight to the point.

pkg/vault/contracts/CompositeLiquidityRouter.sol

pkg/vault/test/foundry/CompositeLiquidityRouterERC4626Pool.t.sol

Co-authored-by: EndymionJkb <[email protected]>

…ttps://github.com/balancer/balancer-v3-monorepo into using-wrapped-amount-in-erc4626-operations-in-clr

…n-clr

EndymionJkb

Looks good now; have one last (very minor) rename suggestion, and some comments.

pkg/interfaces/contracts/vault/ICompositeLiquidityRouter.sol

pkg/vault/contracts/CompositeLiquidityRouter.sol

pkg/vault/test/foundry/CompositeLiquidityRouterERC4626Pool.t.sol

joaobrunoah

LGTM!

jubeira

A bit late, but for the record the latest changes are looking good. Great job @elshan-eth !

fist iteration

1ca2458

elshan-eth requested review from jubeira, joaobrunoah and EndymionJkb December 24, 2024 15:46

jubeira reviewed Jan 6, 2025

View reviewed changes

fix code and tests

757545a

elshan-eth requested a review from jubeira January 7, 2025 18:07

change var name

9dbf7fc

jubeira reviewed Jan 8, 2025

View reviewed changes

refactoring prepareTokens

4b39b72

elshan-eth changed the title ~~Using wrappedToken in CompositeLiquidityRouter in ERC4626Pool operations~~ [IN PROGRESS] Using wrappedToken in CompositeLiquidityRouter in ERC4626Pool operations Jan 9, 2025

fixes

f6bcd4e

elshan-eth changed the title ~~[IN PROGRESS] Using wrappedToken in CompositeLiquidityRouter in ERC4626Pool operations~~ Using wrappedToken in CompositeLiquidityRouter in ERC4626Pool operations Jan 10, 2025

elshan-eth requested a review from jubeira January 10, 2025 13:34

EndymionJkb reviewed Jan 11, 2025

View reviewed changes

elshan-eth and others added 14 commits January 13, 2025 18:10

Update pkg/interfaces/contracts/vault/ICompositeLiquidityRouter.sol

413315d

Co-authored-by: EndymionJkb <[email protected]>

Update pkg/interfaces/contracts/vault/ICompositeLiquidityRouter.sol

c441933

Co-authored-by: EndymionJkb <[email protected]>

Update pkg/interfaces/contracts/vault/ICompositeLiquidityRouter.sol

164cdae

Co-authored-by: EndymionJkb <[email protected]>

Update pkg/interfaces/contracts/vault/ICompositeLiquidityRouter.sol

6d7f538

Co-authored-by: EndymionJkb <[email protected]>

Update pkg/interfaces/contracts/vault/ICompositeLiquidityRouter.sol

c4a2662

Co-authored-by: EndymionJkb <[email protected]>

Update pkg/interfaces/contracts/vault/ICompositeLiquidityRouter.sol

6b1c8db

Co-authored-by: EndymionJkb <[email protected]>

Update pkg/vault/contracts/CompositeLiquidityRouter.sol

b0eeb6e

Co-authored-by: EndymionJkb <[email protected]>

Update pkg/vault/contracts/CompositeLiquidityRouter.sol

6008eca

Co-authored-by: EndymionJkb <[email protected]>

Update pkg/interfaces/contracts/vault/ICompositeLiquidityRouter.sol

06ab630

Co-authored-by: EndymionJkb <[email protected]>

Update pkg/vault/contracts/CompositeLiquidityRouter.sol

9fe3242

Co-authored-by: EndymionJkb <[email protected]>

small fixes

acac9ec

Merge branch 'using-wrapped-amount-in-erc4626-operations-in-clr' of h…

8a876c8

…ttps://github.com/balancer/balancer-v3-monorepo into using-wrapped-amount-in-erc4626-operations-in-clr

fixes

cf43702

update snapshots

af63d70

elshan-eth requested a review from EndymionJkb January 13, 2025 18:26

EndymionJkb reviewed Jan 13, 2025

View reviewed changes

small fixes

8ca5aa6

elshan-eth requested review from EndymionJkb and joaobrunoah January 14, 2025 17:36

small fixes

537f45f

EndymionJkb reviewed Jan 14, 2025

View reviewed changes

jubeira reviewed Jan 15, 2025

View reviewed changes

jubeira reviewed Jan 16, 2025

View reviewed changes

pkg/vault/test/foundry/CompositeLiquidityRouterERC4626Pool.t.sol Show resolved Hide resolved

elshan-eth and others added 11 commits January 17, 2025 16:18

Update pkg/vault/contracts/CompositeLiquidityRouter.sol

7119580

Co-authored-by: EndymionJkb <[email protected]>

Update pkg/vault/contracts/CompositeLiquidityRouter.sol

7828184

Co-authored-by: EndymionJkb <[email protected]>

Update pkg/interfaces/contracts/vault/ICompositeLiquidityRouter.sol

853d161

Co-authored-by: EndymionJkb <[email protected]>

Update pkg/interfaces/contracts/vault/ICompositeLiquidityRouter.sol

af362c2

Co-authored-by: EndymionJkb <[email protected]>

Update pkg/interfaces/contracts/vault/ICompositeLiquidityRouter.sol

fafa55e

Co-authored-by: EndymionJkb <[email protected]>

Update pkg/interfaces/contracts/vault/ICompositeLiquidityRouter.sol

e54e485

Co-authored-by: EndymionJkb <[email protected]>

fix comments

21c8a88

Merge branch 'using-wrapped-amount-in-erc4626-operations-in-clr' of h…

9f07b85

…ttps://github.com/balancer/balancer-v3-monorepo into using-wrapped-amount-in-erc4626-operations-in-clr

fix tests

2a58957

add rest tests

0748090

Merge branch 'main' into using-wrapped-amount-in-erc4626-operations-i…

c5fc0e3

…n-clr

elshan-eth requested review from jubeira and EndymionJkb January 17, 2025 23:26

EndymionJkb approved these changes Jan 18, 2025

View reviewed changes

fix naming

c2943ca

joaobrunoah approved these changes Jan 20, 2025

View reviewed changes

elshan-eth merged commit a7b28cb into main Jan 21, 2025
14 checks passed

elshan-eth deleted the using-wrapped-amount-in-erc4626-operations-in-clr branch January 21, 2025 15:44

jubeira reviewed Jan 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using wrappedToken in CompositeLiquidityRouter in ERC4626Pool operations #1201

Using wrappedToken in CompositeLiquidityRouter in ERC4626Pool operations #1201

elshan-eth commented Dec 24, 2024 •

edited

Loading

jubeira left a comment •

edited

Loading

jubeira left a comment

EndymionJkb left a comment •

edited

Loading

EndymionJkb left a comment

elshan-eth commented Jan 14, 2025

EndymionJkb left a comment

jubeira commented Jan 15, 2025

EndymionJkb commented Jan 15, 2025

jubeira left a comment

EndymionJkb left a comment

joaobrunoah left a comment

jubeira left a comment

Using wrappedToken in CompositeLiquidityRouter in ERC4626Pool operations #1201

Using wrappedToken in CompositeLiquidityRouter in ERC4626Pool operations #1201

Conversation

elshan-eth commented Dec 24, 2024 • edited Loading

Description

Type of change

Checklist:

Issue Resolution

jubeira left a comment • edited Loading

Choose a reason for hiding this comment

jubeira left a comment

Choose a reason for hiding this comment

EndymionJkb left a comment • edited Loading

Choose a reason for hiding this comment

EndymionJkb left a comment

Choose a reason for hiding this comment

elshan-eth commented Jan 14, 2025

EndymionJkb left a comment

Choose a reason for hiding this comment

jubeira commented Jan 15, 2025

EndymionJkb commented Jan 15, 2025

jubeira left a comment

Choose a reason for hiding this comment

EndymionJkb left a comment

Choose a reason for hiding this comment

joaobrunoah left a comment

Choose a reason for hiding this comment

jubeira left a comment

Choose a reason for hiding this comment

elshan-eth commented Dec 24, 2024 •

edited

Loading

jubeira left a comment •

edited

Loading

EndymionJkb left a comment •

edited

Loading